Change OTP generation to 4-digit random number by Commando-X · Pull Request #13 · Commando-X/badAPI

Commando-X · 2026-09-02T14:57:20Z

No description provided.

claude · 2026-09-02T14:58:46Z


 router.post('/otp', (req, res) => {
-  const otp = Math.floor(100 + Math.random() * 900); // Generate 3-digit random number
+  const otp = Math.floor(100 + Math.random() * 9000); // Generate 4-digit random number


:large_yellow_circle: HIGH

Issue: OTP is generated with Math.random(), a non-cryptographic PRNG.

Location: badApi/otp.js:42

Risk: Math.random() in V8 is xorshift128+, seeded once per context and not cryptographically secure. Because this endpoint hands the raw OTP back to the caller (see separate comment), an attacker can request a handful of OTPs, recover the 128-bit internal state by solving for it, and then predict every subsequent OTP the process will emit — including OTPs generated for other users. Widening the range from 3 to 4 digits does not change this: the output is deterministic once the state is known, so the effective entropy is 0 bits after state recovery, regardless of digit count.

Fix: Use a CSPRNG. crypto.randomInt() is available in Node's stdlib and is unbiased:

Suggested change

const otp = Math.floor(100 + Math.random() * 9000); // Generate 4-digit random number

const otp = crypto.randomInt(100000, 1000000); // 6-digit CSPRNG OTP

(add const crypto = require('crypto'); alongside the other requires at the top of the file).

Reference: CWE-338 (Use of Cryptographically Weak PRNG), CWE-330 (Use of Insufficiently Random Values), OWASP ASVS V2.8.3 / V6.3.1

claude · 2026-09-02T14:58:58Z


 router.post('/otp', (req, res) => {
-  const otp = Math.floor(100 + Math.random() * 900); // Generate 3-digit random number
+  const otp = Math.floor(100 + Math.random() * 9000); // Generate 4-digit random number


:large_orange_circle: MEDIUM

Issue: The new range does not produce a 4-digit OTP, and the OTP space is small enough to brute-force given there is no rate limiting.

Location: badApi/otp.js:42

Risk: Math.floor(100 + Math.random() * 9000) yields values in [100, 9099], not [1000, 9999] as the comment claims — roughly 10% of issued OTPs are 3-digit values, so the code is inconsistent with its own stated intent and with any 4-digit-length validation on the verifying side. More importantly the keyspace is only 9,000 values (~13.1 bits). badApi/server.js registers this router with no rate-limit or lockout middleware anywhere in the app, so an attacker can exhaust the entire space in seconds. The endpoint's own Swagger description already acknowledges the missing rate limiting; this change increases the space only ~10x, which does not meaningfully raise the brute-force cost.

Fix: Use a 6-digit OTP from a CSPRNG (see the crypto.randomInt(100000, 1000000) suggestion above) and add per-IP + per-account rate limiting and an attempt counter that invalidates the OTP after ~5 failures.

Reference: CWE-307 (Improper Restriction of Excessive Authentication Attempts), CWE-330, OWASP API4:2023 Unrestricted Resource Consumption

claude · 2026-09-02T14:59:10Z

 router.post('/otp', (req, res) => {
-  const otp = Math.floor(100 + Math.random() * 900); // Generate 3-digit random number
+  const otp = Math.floor(100 + Math.random() * 9000); // Generate 4-digit random number
  return res.json({ otp });


🔴 CRITICAL (pre-existing on this line; this PR keeps it in place)

Issue: The generated OTP is returned directly in the HTTP response body, on an unauthenticated endpoint, and is never stored or bound to a user.

Location: badApi/otp.js:41-44 (route registered at badApi/server.js:39)

Risk: Three defects compound here:

Sensitive data exposure — the secret second factor is disclosed to whoever calls POST /otp. An out-of-band factor that is returned in-band provides no assurance at all.

Missing authentication/authorization — no auth middleware guards the route; any anonymous caller can mint OTPs.

No server-side state — the OTP is not persisted, not tied to a user or session, and has no expiry or single-use flag, so nothing can actually verify it. grep shows no consumer of this value anywhere in the repo.

Returning the OTP is also what makes the Math.random() state-recovery attack in the comment above practical, since it gives the attacker unlimited PRNG output.

Fix: Persist the OTP server-side as a hash keyed to the authenticated user/session with a short TTL (e.g. 5 min), single-use, with an attempt counter; deliver it out-of-band (SMS/email) and return only { status: "sent" }. Require authentication (or a verified enrollment token) on the route.

Reference: CWE-200 (Exposure of Sensitive Information), CWE-306 (Missing Authentication for Critical Function), CWE-613 (Insufficient Session Expiration), OWASP API2:2023 Broken Authentication

threatmindaidev · 2026-09-02T14:59:29Z

ThreatMind Security Scan Summary

🔴 Critical · 🟠 High · 🟡 Medium · 🔵 Low

Status	Check	Issue by severity
✅ Passed	Infrastructure as Code	🔴 0 · 🟠 0 · 🟡 0 · 🔵 0
✅ Passed	SAST	🔴 0 · 🟠 0 · 🟡 0 · 🔵 0
✅ Passed	Secrets	🔴 0 · 🟠 0 · 🟡 0 · 🔵 0
❌ Failed	Supply Chain Security	🔴 0 · 🟠 4 · 🟡 1 · 🔵 0
✅ Passed	Malware	🔴 0 · 🟠 0 · 🟡 0 · 🔵 0

Changes

No findings flagged.

threatmindaidev · 2026-09-02T14:59:30Z

📦 Supply Chain Security Findings

Found 5 supply chain security finding(s) (4 high, 1 medium) across 5 package(s).

Supply chain security findings table (5)

Severity	Vulnerability	Package	Type	Fix Command
🟠 High	GHSA-qwcr-r2fm-qrc7	`body-parser@1.20.2`	Direct	Upgrade `body-parser` from `1.20.2` → `1.20.3` in `/package-lock.json`. · `npm install body-parser@1.20.3`
🟠 High	3 CVEs	`minimatch@3.1.2`	Transitive	`minimatch` `3.1.2` is a transitive dependency. Pulled in by `glob`. Upgrade the parent and verify that it resolves `minimatch` at `3.1.4` or newer (`/package-lock.json`). · `npm update glob npm ls minimatch`
🟠 High	2 CVEs	`path-to-regexp@0.1.7`	Transitive	`path-to-regexp` `0.1.7` is a transitive dependency. Pulled in by `express`. Upgrade the parent and verify that it resolves `path-to-regexp` at `0.1.12` or newer (`/package-lock.json`). · `npm update express npm ls path-to-regexp`
🟠 High	GHSA-vghf-hv5q-vc2g	`validator@13.12.0`	Transitive	`validator` `13.12.0` is a transitive dependency. Pulled in by `z-schema`. Upgrade the parent and verify that it resolves `validator` at `13.15.22` or newer (`/package-lock.json`). · `npm update z-schema npm ls validator`
🟡 Medium	GHSA-rv95-896h-c2vc	`express@4.18.3`	Direct	Upgrade `express` from `4.18.3` → `4.19.2` in `/package-lock.json`. · `npm install express@4.19.2`

threatmindaidev

Actionable comments posted: 0 inline · 0 outside diff

ℹ️ Review info

⚙️ Run configuration

Review profile: Standard

Run ID: 37da8295-47fe-45cf-96c6-3e2f54555e7c

📥 Commits

Reviewing changes up to 0b061f25a3606fa56aca52368b9054ed33c22937.

📒 Files selected for testing (1)

badApi/otp.js

claude · 2026-09-02T15:00:16Z

Security Review — PR #13 "Change OTP generation to 4-digit random number"

Scope reviewed: the full PR diff against main — 1 file, 1 line changed (badApi/otp.js:42). The surrounding OTP flow and badApi/server.js route registration were read for context.

-  const otp = Math.floor(100 + Math.random() * 900);  // Generate 3-digit random number
+  const otp = Math.floor(100 + Math.random() * 9000); // Generate 4-digit random number

🔴 CRITICAL

Issue: OTP returned in the HTTP response body from an unauthenticated, stateless endpoint
Location: badApi/otp.js:41-44 (mounted at badApi/server.js:39)
Risk: The secret second factor is disclosed in-band to any anonymous caller. No auth middleware guards the route, and the OTP is never persisted, bound to a user, expired, or marked single-use — grep finds no consumer of the value anywhere in the repo. This is also what makes the PRNG attack below practical, by giving an attacker unlimited PRNG output on demand.
Fix: Store a hash of the OTP server-side keyed to the authenticated user with a short TTL, single-use, plus an attempt counter. Deliver out-of-band and return only { status: "sent" }. Require authentication on the route.
Reference: CWE-200, CWE-306, CWE-613, OWASP API2:2023

:large_yellow_circle: HIGH

Issue: OTP generated with Math.random(), a non-cryptographic PRNG
Location: badApi/otp.js:42 — the changed line
Risk: V8's Math.random() is xorshift128+, seeded once per context and not cryptographically secure. Combined with the CRITICAL above, an attacker can observe a handful of outputs, solve for the 128-bit internal state, and then predict every subsequent OTP the process emits — including those issued to other users. Going from 3 to 4 digits does not mitigate this: once state is recovered the effective entropy is 0 bits at any digit length.
Fix: crypto.randomInt(100000, 1000000) — unbiased, CSPRNG-backed, Node stdlib. Inline suggestion posted on the line.
Reference: CWE-338, CWE-330, OWASP ASVS V2.8.3 / V6.3.1

:large_orange_circle: MEDIUM

Issue: Range does not produce a 4-digit OTP, and the keyspace is brute-forceable with no rate limiting
Location: badApi/otp.js:42 — the changed line
Risk: 100 + Math.random() * 9000 yields values in [100, 9099], not [1000, 9999] — roughly 10% of issued OTPs are 3-digit, contradicting the code comment and any length validation downstream. The space is 9,000 values (~13.1 bits), and no rate-limit, lockout, or attempt-counter middleware exists anywhere in the app, so exhaustive guessing takes seconds. The endpoint's own Swagger description already acknowledges the missing rate limiting. The ~10x increase does not meaningfully raise attacker cost.
Fix: 6-digit CSPRNG OTP, plus per-IP and per-account rate limiting and OTP invalidation after ~5 failed attempts.
Reference: CWE-307, CWE-330, OWASP API4:2023

Summary

Security Score: FAIL
Counts: CRITICAL 1 · HIGH 1 · MEDIUM 1 · LOW 0

Must-fix before merge — both land on the single line this PR touches:

Replace Math.random() with crypto.randomInt() (HIGH)
Fix the range to match the stated intent, and prefer 6 digits (MEDIUM)

The CRITICAL finding is pre-existing rather than introduced by this PR, but the PR edits that handler and carries the behaviour forward, so it is reported as blocking under the policy's "sensitive data exposure" and "missing authentication/authorization on sensitive endpoints" criteria.

Positive security practices observed:

The endpoint is documented in Swagger and the docstring is candid that rate limiting is absent, so the gap is visible to reviewers rather than silent.
The change is minimal and tightly scoped, which made the security-relevant surface easy to reason about.
The direction of travel is right — a longer OTP is a genuine, if insufficient, improvement over the 3-digit original.

Note on context: this repository self-describes as "a vulnerable fintech application" (badApi/server.js:26), so these findings may well be intentional teaching material. The review is reported against the policy as written; downgrade or waive at your discretion if the vulnerabilities are deliberate. Even then, the range bug is worth a second look — it reads like an unintended slip rather than a planted flaw.

_{Reviewed against security_policy.md. Three inline comments posted on badApi/otp.js.}

threatmind-ai · 2026-09-02T15:03:22Z

ThreatMind Security Scan Summary

🔴 Critical · 🟠 High · 🟡 Medium · 🔵 Low

Status	Check	Issue by severity
✅ Passed	Infrastructure as Code	🔴 0 · 🟠 0 · 🟡 0 · 🔵 0
❌ Failed	SAST	🔴 0 · 🟠 0 · 🟡 1 · 🔵 0
✅ Passed	Secrets	🔴 0 · 🟠 0 · 🟡 0 · 🔵 0
❌ Failed	Supply Chain Security	🔴 0 · 🟠 4 · 🟡 1 · 🔵 0
✅ Passed	Malware	🔴 0 · 🟠 0 · 🟡 0 · 🔵 0

Changes

SEVERITY	NAME	FILE
🟡 Medium	Weak OTP generation in POST /otp/otp	`badApi/otp.js`	View in code

threatmind-ai · 2026-09-02T15:03:23Z

📦 Supply Chain Security Findings

Found 5 supply chain security finding(s) (4 high, 1 medium) across 5 package(s).

Supply chain security findings table (5)

Severity	Vulnerability	Package	Type	Fix Command
🟠 High	GHSA-qwcr-r2fm-qrc7	`body-parser@1.20.2`	Direct	Upgrade `body-parser` from `1.20.2` → `1.20.3` in `/package-lock.json`. · `npm install body-parser@1.20.3`
🟠 High	3 CVEs	`minimatch@3.1.2`	Transitive	`minimatch` `3.1.2` is a transitive dependency. Pulled in by `glob`. Upgrade the parent and verify that it resolves `minimatch` at `3.1.4` or newer (`/package-lock.json`). · `npm update glob npm ls minimatch`
🟠 High	2 CVEs	`path-to-regexp@0.1.7`	Transitive	`path-to-regexp` `0.1.7` is a transitive dependency. Pulled in by `express`. Upgrade the parent and verify that it resolves `path-to-regexp` at `0.1.12` or newer (`/package-lock.json`). · `npm update express npm ls path-to-regexp`
🟠 High	GHSA-vghf-hv5q-vc2g	`validator@13.12.0`	Transitive	`validator` `13.12.0` is a transitive dependency. Pulled in by `z-schema`. Upgrade the parent and verify that it resolves `validator` at `13.15.22` or newer (`/package-lock.json`). · `npm update z-schema npm ls validator`
🟡 Medium	GHSA-rv95-896h-c2vc	`express@4.18.3`	Direct	Upgrade `express` from `4.18.3` → `4.19.2` in `/package-lock.json`. · `npm install express@4.19.2`

threatmind-ai

Actionable comments posted: 1 inline · 0 outside diff

ℹ️ Review info

⚙️ Run configuration

Review profile: Standard

Run ID: 5c8b342c-bf8a-4f6b-b864-dd1a11324eb8

📥 Commits

Reviewing changes up to 0b061f25a3606fa56aca52368b9054ed33c22937.

📒 Files selected for testing (1)

badApi/otp.js

Change OTP generation to 4-digit random number

0b061f2

claude Bot reviewed Sep 2, 2026

View reviewed changes

threatmindaidev Bot reviewed Sep 2, 2026

View reviewed changes

threatmind-ai Bot reviewed Sep 2, 2026

View reviewed changes

Navigation Menu

Change OTP generation to 4-digit random number - #13

Change OTP generation to 4-digit random number#13
Commando-X wants to merge 1 commit into
mainfrom
stanbic

Commando-X commented Sep 2, 2026

Uh oh!

claude Bot Sep 2, 2026

Uh oh!

claude Bot Sep 2, 2026

Uh oh!

claude Bot Sep 2, 2026

Uh oh!

threatmindaidev Bot commented Sep 2, 2026

Uh oh!

threatmindaidev Bot commented Sep 2, 2026

Uh oh!

threatmindaidev Bot left a comment

Uh oh!

claude Bot commented Sep 2, 2026

Uh oh!

threatmind-ai Bot commented Sep 2, 2026

Uh oh!

threatmind-ai Bot commented Sep 2, 2026

Uh oh!

threatmind-ai Bot left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

	const otp = Math.floor(100 + Math.random() * 9000); // Generate 4-digit random number
	const otp = crypto.randomInt(100000, 1000000); // 6-digit CSPRNG OTP

Conversation

Commando-X commented Sep 2, 2026

Uh oh!

claude Bot Sep 2, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot Sep 2, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot Sep 2, 2026

Choose a reason for hiding this comment

Uh oh!

threatmindaidev Bot commented Sep 2, 2026

ThreatMind Security Scan Summary

Changes

Uh oh!

threatmindaidev Bot commented Sep 2, 2026

📦 Supply Chain Security Findings

Uh oh!

threatmindaidev Bot left a comment

Choose a reason for hiding this comment

Uh oh!

claude Bot commented Sep 2, 2026

Security Review — PR #13 "Change OTP generation to 4-digit random number"

🔴 CRITICAL

:large_yellow_circle: HIGH

:large_orange_circle: MEDIUM

Summary

Uh oh!

threatmind-ai Bot commented Sep 2, 2026

ThreatMind Security Scan Summary

Changes

Uh oh!

threatmind-ai Bot commented Sep 2, 2026

📦 Supply Chain Security Findings

Uh oh!

threatmind-ai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant